Open
Conversation
Introduce a new Models-as-Data sink sub-kind path-injection[read] for models that only read from or inspect a path. The general java/path-injection query and its PathInjectionSanitizer barrier continue to consider both path-injection and path-injection[read] sinks, so no alerts are lost. The java/zipslip query deliberately selects only path-injection sinks, since read-only accesses such as ClassLoader.getResource or FileInputStream are outside the archive extraction threat model. Addresses github#21606 along the lines proposed on the issue thread: prefer path-injection[read] over a [create] sub-kind so that miscategorizing a sink causes a false positive (easy to spot) rather than a false negative. - shared/mad/codeql/mad/ModelValidation.qll: allow path-injection[...] as a valid sink kind. - java/ql/lib/ext/*.model.yml: relabel the models that PR github#12916 migrated from the historical read-file kind (plus the newer ClassLoader resource-lookup variants that share the same read-only semantics). - java/ql/lib/semmle/code/java/security/TaintedPathQuery.qll and PathSanitizer.qll: select both path-injection and path-injection[read] sinks/barriers. - java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll: keep only path-injection, with a comment explaining why path-injection[read] is excluded. - java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.java: add m7 regression covering the Dubbo-style classpath lookup from issue github#21606 and assert no alert is produced. - Update TaintedPath.expected for the renamed kinds in the models list. - Add change-notes under java/ql/lib/change-notes and java/ql/src/change-notes.
The sink-model generator and the experimental java/file-path-injection query now observe the new path-injection[read] sub-kind for the FileInputStream and Files.copy source-argument models. - CWE-073 FilePathInjection.expected: refresh the models table for the renamed kind on FileInputStream(File); alerts unchanged. - modelgenerator Sinks.java: update the inline sink annotation for copyFileToDirectory(Path,Path,CopyOption[]) Argument[0] to the new path-injection[read] sub-kind, mirroring the library change.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a read-only sub-kind for path injection sinks to reduce java/zipslip false positives by separating “read-only path usage” from “file creation/extraction” path usage.
Changes:
- Added a new sink sub-kind
path-injection[read]and updated sink-kind validation to acceptpath-injection[%]. - Relabeled Java Models-as-Data sink rows for read-only APIs from
path-injectiontopath-injection[read], and updatedjava/path-injection-related query logic to include both kinds whilejava/zipslipstays onpath-injectiononly. - Updated regression tests, expected outputs, and change notes to reflect the new sink kind and the ZipSlip FP reduction.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/mad/codeql/mad/ModelValidation.qll | Accepts path-injection[%] as a valid sink kind and documents the intent of path-injection[read]. |
| java/ql/test/utils/modelgenerator/dataflow/p/Sinks.java | Updates model-generator inline sink annotation to use path-injection[read] for read-side copy argument. |
| java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.java | Adds regression coverage ensuring read-only resource/file-open uses are // OK for ZipSlip. |
| java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected | Updates expected model kind strings to path-injection[read] for relabeled sinks. |
| java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected | Updates expected model kind strings to path-injection[read] for relabeled sinks. |
| java/ql/src/change-notes/2026-04-21-zipslip-exclude-read-sinks.md | Notes the ZipSlip FP reduction via excluding path-injection[read]. |
| java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll | Documents/ensures ZipSlip selects only path-injection sinks. |
| java/ql/lib/semmle/code/java/security/TaintedPathQuery.qll | Updates java/path-injection sink selection to include both path-injection and path-injection[read]. |
| java/ql/lib/semmle/code/java/security/PathSanitizer.qll | Applies external path-injection sanitizers to both sink kinds. |
| java/ql/lib/ext/java.io.model.yml | Relabels read-only constructors (e.g., FileInputStream, FileReader) to path-injection[read]. |
| java/ql/lib/ext/java.lang.model.yml | Relabels classloader/resource lookup sinks to path-injection[read]. |
| java/ql/lib/ext/java.nio.file.model.yml | Relabels read-only Files.* APIs (copy-from, read, lines, readers/streams) to path-injection[read]. |
| java/ql/lib/ext/javax.servlet.model.yml | Relabels ServletContext.getResourceAsStream to path-injection[read]. |
| java/ql/lib/ext/com.google.common.io.model.yml | Relabels Guava read-oriented file APIs to path-injection[read]. |
| java/ql/lib/ext/com.thoughtworks.xstream.model.yml | Relabels XStream.fromXML(File) as path-injection[read]. |
| java/ql/lib/ext/org.apache.commons.io.model.yml | Relabels FileUtils.openInputStream(File) to path-injection[read]. |
| java/ql/lib/ext/org.apache.commons.net.model.yml | Relabels key-manager file path inputs to path-injection[read]. |
| java/ql/lib/ext/org.apache.tools.ant.model.yml | Relabels Ant classpath/basedir-style read-oriented sinks to path-injection[read]. |
| java/ql/lib/ext/org.apache.tools.ant.taskdefs.model.yml | Relabels Ant read-oriented inputs (e.g., Expand.setSrc) to path-injection[read]. |
| java/ql/lib/ext/org.kohsuke.stapler.framework.io.model.yml | Relabels LargeText(File,...) to path-injection[read]. |
| java/ql/lib/ext/org.springframework.util.model.yml | Relabels the read-side argument of FileCopyUtils.copy(File,File) to path-injection[read]. |
| java/ql/lib/ext/kotlin.io.model.yml | Relabels Kotlin FilesKt read methods to path-injection[read]. |
| java/ql/lib/ext/io.netty.handler.stream.model.yml | Relabels Netty ChunkedFile file argument to path-injection[read]. |
| java/ql/lib/ext/io.netty.handler.ssl.model.yml | Relabels Netty SSL file inputs to path-injection[read]. |
| java/ql/lib/ext/io.netty.handler.codec.http.multipart.model.yml | Relabels file-upload input file to path-injection[read]. |
| java/ql/lib/ext/hudson.model.yml | Relabels read-side FilePath operations to path-injection[read]. |
| java/ql/lib/ext/hudson.model.model.yml | Relabels read-side Hudson model operations to path-injection[read]. |
| java/ql/lib/ext/hudson.scm.model.yml | Relabels read-side SCM operations to path-injection[read]. |
| java/ql/lib/ext/hudson.util.model.yml | Relabels read-side Hudson utility operations to path-injection[read]. |
| java/ql/lib/ext/hudson.util.jna.model.yml | Relabels GNUCLibrary.open path argument to path-injection[read]. |
| java/ql/lib/change-notes/2026-04-21-path-injection-read-subkind.md | Documents the new path-injection[read] sink kind. |
Comment on lines
+57
to
+60
| // shared: path-injection[read] identifies sinks that only read from a path | ||
| // (e.g. ClassLoader.getResource, FileInputStream, File.exists). Queries such | ||
| // as java/zipslip that only care about write/extraction deliberately exclude | ||
| // this sub-kind. |
There was a problem hiding this comment.
The new comment uses File.exists as an example of a path-injection[read] sink, but java.io.File.exists() is still modeled as plain path-injection (and therefore not part of the new sub-kind). Please either (a) update the example list to only mention APIs that are actually labeled path-injection[read] (e.g., ClassLoader.getResource, FileInputStream, Files.readAllBytes), or (b) move File.exists/similar existence-check models to path-injection[read] if the intent is to treat them as read-only sinks going forward.
Comment on lines
+58
to
+59
| * read-only path sinks (e.g. `ClassLoader.getResource`, `FileInputStream`, | ||
| * `File.exists`) are outside the threat model. |
There was a problem hiding this comment.
This doc comment says File.exists is excluded by selecting only path-injection, but java.io.File.exists() (and also java.nio.file.Files.exists(...)) are still modeled as path-injection, so ZipSlip will still treat flows into those calls as sinks. Please either adjust the examples here to match what is actually excluded (for example, ClassLoader.getResource, FileInputStream, FileReader), or relabel the exists models to path-injection[read] if you want ZipSlip to exclude them too.
Suggested change
| * read-only path sinks (e.g. `ClassLoader.getResource`, `FileInputStream`, | |
| * `File.exists`) are outside the threat model. | |
| * read-only path sinks (for example `ClassLoader.getResource`, | |
| * `FileInputStream`, and `FileReader`) are outside the threat model. |
Contributor
Since these models were added after the two sink kinds were merged, please apply your best judgement to decide if they should be |
owen-mc
reviewed
Apr 22, 2026
Comment on lines
+57
to
+61
| // shared: path-injection[read] identifies sinks that only read from a path | ||
| // (e.g. ClassLoader.getResource, FileInputStream, File.exists). Queries such | ||
| // as java/zipslip that only care about write/extraction deliberately exclude | ||
| // this sub-kind. | ||
| "path-injection[%]", |
Contributor
There was a problem hiding this comment.
Please mark this as "Java-only currently" instead of "shared" and make the rest of the comment shorter. There is no need for example APIs in the comment, or specific java query ids.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #21606.
As reported in #21606,
java/zipslipflags archive-entry names that flow only into read-only lookups such asClassLoader.getSystemResources, which are outside the Zip Slip threat model (no file extraction happens). Our earlier attempt in #21609 narrowed the sink set inside the ZipSlip query itself. In #21606 (comment) and #21606 (comment) @owen-mc suggested a cleaner MaD-level fix instead, which this PR implements:path-injection[read].read-file(merged intopath-injectionin Java: revamp MaD sink kinds #12916) topath-injection[read].java/path-injection) select both kinds; queries that only care about file creation (java/zipslip) select onlypath-injectionand deliberately excludepath-injection[read].Per @owen-mc's design rationale,
[read]is the narrow sub-kind so that mis-labeling a new sink causes a false positive (easy to spot) rather than a false negative (hard to spot). #21609 should be closed in favour of this PR.Changes
Sink-kind validation
shared/mad/codeql/mad/ModelValidation.qll: acceptpath-injection[%]as a valid sink kind.Relabel read-only models
Changed 45 rows across 21 Java MaD yml files from
path-injectiontopath-injection[read]. The scope is the historicalread-fileset from #12916 plus the ClassLoader resource-lookup variants added later (Class.getResource,ClassLoader.getResource,getResources,getSystemResources, etc.) that share the same read-only semantics and motivate the Dubbo example in #21606. Notable files:java/ql/lib/ext/java.io.model.yml:FileInputStream,FileReaderconstructors.java/ql/lib/ext/java.lang.model.yml:Class.getResource*,ClassLoader.getResource*/getResources/getSystemResource*,Module.getResourceAsStream.java/ql/lib/ext/java.nio.file.model.yml:Files.copy(Path,_)source argument,Files.lines,newBufferedReader,newInputStream,readAllBytes,readAllLines,readString.com.google.common.io.Files,XStream, Hudson/JenkinsFilePath, Netty SSL/stream, Ant, Spring,javax.servlet,org.apache.commons.io,kotlin.io.Entries that delete, modify, or write (
File.delete,File.setReadable,Files.newOutputStream,FileOutputStream,createNewFile,createDirectory,renameTo, etc.) stay as plainpath-injection.File.canRead,File.exists,File.isDirectory, etc. are left aspath-injectionfor this PR since they were never part of the historicalread-fileset. They can be moved to[read]later if reviewers prefer.Query updates
java/ql/lib/semmle/code/java/security/TaintedPathQuery.qll: select bothpath-injectionandpath-injection[read]sojava/path-injectionbehavior is unchanged.java/ql/lib/semmle/code/java/security/PathSanitizer.qll: the external-MaDPathInjectionSanitizeralso applies to both kinds, so existing barrier models keep working across both queries.java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll: stays onpath-injectiononly, with an explanatory comment about the deliberate exclusion ofpath-injection[read].Tests
java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.javagainsm7, a regression mirroring the Dubbo classpath-lookup case from False positive:java/zipslipflags archive entry names used for class loading, not file extraction #21606: archive entry names flowing intoClassLoader.getSystemResources,getClass().getResource(AsStream),FileInputStream, andFileReaderare all asserted// OK.TaintedPath.expected,FilePathInjection.expected, and the modelgeneratorSinks.javainline annotation are updated for the renamed kind strings. Alert sets are unchanged.Change notes
java/ql/lib/change-notes/2026-04-21-path-injection-read-subkind.mddocuments the new sub-kind.java/ql/src/change-notes/2026-04-21-zipslip-exclude-read-sinks.mddocuments the FP reduction injava/zipslip.Test plan
codeql test run java/ql/test/query-tests/security/CWE-022/— ZipSlip (incl. the newm7) and TaintedPath pass.codeql test run java/ql/test/library-tests/dataflow/external-models/—validatemodels.qlpasses, confirmingpath-injection[read]is a valid kind.codeql test run java/ql/test/utils/modelgenerator/dataflow/— sink-model generator and baseline annotations agree.codeql test run java/ql/test/query-tests/security/CWE-643/,.../CWE-200/.../TempDirLocalInformationDisclosure/,java/ql/test/experimental/query-tests/security/CWE-073/,.../CWE-200/all pass.codeql query compileonZipSlip.qlandTaintedPath.ql.